Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client: don't call callbacks if close() is called #258

Merged
merged 1 commit into from
Oct 21, 2018

Conversation

virtuald
Copy link
Contributor

  • This could break some existing clients using the raw grpc API (maybe should document it?), but ts-protoc-gen clears out the callbacks anyways so this shouldn't cause anyone pain
  • Fixes race condition on stream close #257

... I'd write a test for this, but I'm not actually a Javascript/Typescript developer so I'm not familiar with the test framework.

@virtuald
Copy link
Contributor Author

Worth noting that when I built this PR into my application, the errors I was experiencing went away.

@johanbrandhorst
Copy link
Contributor

This seems reasonable to me, but I'm also not a typescript developer 😂. I've rerun the failing job, the CI tests are a bit flaky unfortunately.

@johanbrandhorst
Copy link
Contributor

This seems to be failing consistently https://travis-ci.org/improbable-eng/grpc-web/jobs/441938330, though I doubt it's related to this change:

/home/travis/gopath/src/github.com/improbable-eng/grpc-web/test/custom-karma-driver.ts:140
        self.localTunnel.dispose(function () {
                         ^
TypeError: Cannot read property 'dispose' of undefined

@johanbrandhorst
Copy link
Contributor

Given that it passes everywhere else, I think it might just be a test configuration error. I can't force this through myself but have tried alerting others so we can get this in.

@virtuald
Copy link
Contributor Author

Ping.

@jonnyreeves
Copy link
Contributor

Just had a look into the build failure, I believe the root cause is the failure to bring up Chrome:

Error: [init({"project":"master","build":"travis_803","acceptSslCerts":true,"defaultVideo":true,"browserstack.local":true,"browserstack.tunnel":true,"browserstack.debug":true,"tunnelIdentifier":"tunnel-0.9422959229946926","browserstack.localIdentifier":"tunnel-0.9422959229946926","testSuite":"client","browserName":"chrome","browserVersion":"42","os":"Windows","os_version":"7"})] The environment you requested was unavailable.
Could not start Browser / Emulator

We are seeing the same failure on master so I am also confident this failure is unrelated to the changes.

I'll merge this through, however we should follow up with fixing this particular browser test, or remove it from the matrix

@jonny-improbable
Copy link
Contributor

@virtuald could you please bring this up to speed with master now #262 was merged? Thanks!

- ts-protoc-gen clears out the callbacks anyways
- Fixes improbable-eng#257
@virtuald
Copy link
Contributor Author

Looks like it passed. 👍

@johanbrandhorst johanbrandhorst merged commit 2974dfa into improbable-eng:master Oct 21, 2018
@johanbrandhorst
Copy link
Contributor

LGTM!

@virtuald virtuald deleted the fix-close-race branch November 21, 2018 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants